Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3e4fde51a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ((l.elevation * {METERS_TO_FEET}) - lo.depth_to_water_below_ground_surface) | ||
| AS water_elevation, |
There was a problem hiding this comment.
In _create_water_elevation_view, water_elevation now converts l.elevation to feet but subtracts depth_to_water_below_ground_surface without unit normalization, so any groundwater observation stored in meters (o.unit = 'm') will produce a mixed-unit result (feet minus meters) and an incorrect elevation. This regression is user-impacting wherever groundwater-level records are not already in feet, so the expression should convert depth based on o.unit (or explicitly constrain to feet-only data).
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def downgrade() -> None: | ||
| op.execute(text("DROP MATERIALIZED VIEW IF EXISTS ogc_water_elevation_wells")) |
There was a problem hiding this comment.
The migration's downgrade() only drops ogc_water_elevation_wells, so rolling back from this revision leaves the database at revision m6f7a8b9c0d1 without the view that revision expects, breaking rollback consistency and any features/commands that query or refresh that materialized view after a downgrade. The downgrade path should recreate the prior view definition instead of deleting it outright.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes a unit mismatch in the ogc_water_elevation_wells materialized view. Previously, the water elevation was computed as elevation - depth_to_water, which mixed meters (elevation from the Location table) with feet (depth-to-water from observations). The fix converts elevation to feet before the subtraction: (elevation_m * 3.28084) - depth_to_water_ft.
Changes:
- New Alembic migration that drops and recreates the
ogc_water_elevation_wellsmaterialized view with the meters-to-feet conversion applied to elevation before computingwater_elevation - Updated test assertion to validate the new formula using the conversion factor
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
alembic/versions/n7a8b9c0d1e2_fix_water_elevation_units_to_feet.py |
New migration that recreates the materialized view with (elevation * 3.28084) - depth_to_water formula for consistent feet output |
tests/test_ogc.py |
Updated expected water elevation calculation to match the new conversion formula |
You can also share your feedback on Copilot code review. Take the survey.
| def downgrade() -> None: | ||
| op.execute(text("DROP MATERIALIZED VIEW IF EXISTS ogc_water_elevation_wells")) |
There was a problem hiding this comment.
The downgrade() only drops the materialized view, but since this migration modifies an existing view (created by m6f7a8b9c0d1), the downgrade should recreate the previous version of the view (the one without the meters-to-feet conversion). Simply dropping the view leaves the database without ogc_water_elevation_wells entirely after a downgrade, which would break any code that depends on it (e.g., cli/cli.py refreshes, pygeoapi collection endpoints, etc.).
The downgrade should drop the current view and then recreate the original view definition from m6f7a8b9c0d1 (using l.elevation - ro.depth_to_water_below_ground_surface without the conversion factor), along with the original comment and unique index.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?